GH-37093: [Python] Add async Flight client with GetFlightInfo #36986

lidavidm · 2023-08-01T20:45:27Z

Rationale for this change

Demonstrate how to deal with async Flight from Python.

What changes are included in this PR?

Add bindings for GetFlightInfo.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes, new APIs.

Closes: [FlightRPC][Python] Implement async GetFlightInfo #37093

lidavidm · 2023-08-09T15:07:30Z

CC @pitrou @jorisvandenbossche this is a skeleton of how to bind async C++ code to asyncio in Python, though I still need to get it working with both asyncio and Trio. (I'm looking at https://github.com/codypiersall/pynng/blob/master/pynng/_aio.py as a reference of how we might do that.)

pitrou

This looks like a good start to me.

python/pyarrow/_flight.pyx

pitrou · 2023-08-09T15:25:35Z

python/pyarrow/_flight.pyx

@@ -1320,6 +1399,11 @@ cdef class FlightClient(_Weakrefable):
            check_flight_status(CFlightClient.Connect(c_location, c_options
                                                      ).Value(&self.client))

+    def as_async(self) -> None:
+        if anyio is None:
+            raise RuntimeError("anyio is required for the async interface")


I'm not sure why we would want anyio. Just start with plain asyncio?

The main thing is to support both asyncio and Trio users (though I think anyio is unnecessary and we can just directly support them both)

Why would we want that, though? asyncio is the de facto standard. And the anyio API for delegating stuff to external threads seems rather convoluted.

I don't see a reason to just lock out a part of the ecosystem.

And FWIW, one of the very first issues opened against ADBC was request for async support via anyio specifically, to support both Trio and asyncio

python/pyarrow/src/arrow/python/flight.cc

lidavidm · 2023-08-09T19:11:10Z

Updated, added basic tests. I'll deal with Trio in a later PR, as well as raising an RpcError-type exception instead of built in exceptions (again: RPC errors should be distinct, and should be handled uniformly), once I've reviewed the C++ side and decided I'm happy with TransportStatusDetail.

There's more we can do with e.g. pytest-asyncio and such (down the line, we'd want all tests to be parametrized on asyncio vs Trio) that I don't think is worth tackling immediately.

python/pyarrow/_flight.pyx

lidavidm · 2023-08-10T12:55:29Z

Hmm, that's an interesting segfault in CI.

lidavidm · 2023-08-10T14:24:10Z

Alright:

The weird SystemError: <class 'SystemError'> returned a result with an error set is inscrutable to me. It occurs along with a TypeError: __traceback__ must be a traceback or None message. with_traceback doesn't appear to be able to clear the problematic traceback and I can't get gdb to break on the source of the error properly.

The segfault is also related to tracebacks, somehow; it reproduces with PYTHONDEVMODE and gdb shows it happens somewhere in tb_traverse, where obj->ob_type points to something that appears to have already been deallocated:

#0  _PyObject_IS_GC (obj=0x7fff656ce7af) at /usr/local/src/conda/python-3.11.3/Include/object.h:772
#1  visit_decref (op=0x7fff656ce7af, parent=parent@entry=0x7fff656ce710)
    at /usr/local/src/conda/python-3.11.3/Modules/gcmodule.c:452
#2  0x000055555583df5b in tb_traverse (tb=0x7fff656ce710, visit=0x555555723a20 <visit_decref>, arg=0x7fff656ce710)
    at /usr/local/src/conda/python-3.11.3/Python/traceback.c:182

lidavidm · 2023-08-10T14:59:47Z

So, I don't understand why, but bouncing through a pure-Python thread fixes the segfault. It seems something about having the C-callback in the traceback at some point borks the interpreter. It does seem that the asyncio implementation tries to get the traceback unconditionally (in call_soon_threadsafe -> call_soon -> asyncio.events.Handle) so I suppose maybe Cython isn't giving a valid traceback object for _client_async_get_flight_info? (Should probably go confirm that.)

No clue about the first error...

pitrou · 2023-08-10T15:36:20Z

Can the segfault be reproduced locally?

lidavidm · 2023-08-10T15:46:31Z

Can the segfault be reproduced locally?

Yes, just run the test with PYTHONDEVMODE. The other error only seems to reproduce under Docker though I haven't tried very hard so far.

pitrou · 2023-08-10T15:51:55Z

Ok, I'll take a look hopefully on Monday.

lidavidm · 2023-08-10T16:36:08Z

Looking at these:

It looks like Cython indeed doesn't generate stack frames for calls, and also modifying wakeup here to try to call sys._getframe like asyncio.event.Handle does crashes with call stack is not deep enough. So indeed, it seems we should avoid touching asyncio or generating exceptions from the native callback. Bouncing through a Python thread seems unavoidable from that perspective, unfortunately. (asyncio tries to generate stacktraces to support async debugging.)

pitrou

Thanks a lot. This really opens interesting possibilities, I think.

pitrou · 2023-08-23T15:41:25Z

python/pyarrow/_flight.pyx

+    async def get_flight_info(
+        self,
+        descriptor: FlightDescriptor,
+        *,
+        options: FlightCallOptions = None,
+    ):
+        call = AsyncioCall()
+        self._get_flight_info(call, descriptor, options)
+        return await call.as_awaitable()


This could just be:

Suggested change

async def get_flight_info(

self,

descriptor: FlightDescriptor,

*,

options: FlightCallOptions = None,

):

call = AsyncioCall()

self._get_flight_info(call, descriptor, options)

return await call.as_awaitable()

def get_flight_info(

self,

descriptor: FlightDescriptor,

*,

options: FlightCallOptions = None,

):

call = AsyncioCall()

self._get_flight_info(call, descriptor, options)

return call.as_awaitable()

but I've kept the explicit async def for introspection and self-documentation... what do you think @jorisvandenbossche ?

pitrou · 2023-08-23T15:50:56Z

At least test-debian-11-python-3 has failed with:
https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=53319&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=d9b15392-e4ce-5e4c-0c8c-b69645229181&l=5448

usr/local/lib/python3.9/dist-packages/pyarrow/tests/test_flight_async.py:59: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
usr/lib/python3.9/asyncio/runners.py:44: in run
    return loop.run_until_complete(main)
usr/lib/python3.9/asyncio/base_events.py:642: in run_until_complete
    return future.result()
usr/local/lib/python3.9/dist-packages/pyarrow/tests/test_flight_async.py:56: in _test
    info = await async_client.get_flight_info(descriptor)
pyarrow/_flight.pyx:1281: in get_flight_info
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   pyarrow.lib.ArrowNotImplementedError: gRPC 1.40 or newer is required to use async

We could perhaps expose supports_async() in Python.

pitrou · 2023-08-23T15:53:45Z

That said, it would perhaps be nicer to expose the same error message in Python as in C++...

…kipping

pitrou · 2023-08-23T16:23:18Z

@github-actions crossbow submit -g python

pitrou · 2023-08-23T16:23:39Z

cpp/src/arrow/flight/client.h

+  /// This is like supports_async(), except that a detailed error message
+  /// is returned if async support is not available.  If async support is
+  /// available, this function returns successfully.
+  Status CheckAsyncSupport() const;


@lidavidm Does this API look ok?

LGTM, though I would be happy to replace supports_async entirely with this.

github-actions · 2023-08-23T16:27:27Z

Revision: 58a938a

Submitted crossbow builds: ursacomputing/crossbow @ actions-b4276e2530

Task	Status
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.4.1
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.4.1
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-python
test-debian-11-python-3
test-fedora-35-python-3
test-ubuntu-20.04-python-3
test-ubuntu-22.04-python-3

pitrou · 2023-08-23T17:33:37Z

CI failures are unrelated.

conbench-apache-arrow · 2023-08-24T21:08:33Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit b6ab909.

There were 4 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-08-24 06:00:55Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1
and 2 more (see the report linked below)

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…pache#36986) ### Rationale for this change Demonstrate how to deal with async Flight from Python. ### What changes are included in this PR? Add bindings for GetFlightInfo. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes, new APIs. * Closes: apache#37093 Lead-authored-by: David Li <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

This comment was marked as resolved.

Sign in to view

github-actions bot added Component: C++ Component: FlightRPC awaiting committer review Awaiting committer review Component: Python labels Aug 1, 2023

lidavidm changed the title ~~WIP: [Python] Add async Flight client with GetFlightInfo~~ GH-37093: [Python] Add async Flight client with GetFlightInfo Aug 9, 2023

lidavidm force-pushed the flight-async-py-client-getflightinfo branch from 25fa26f to 1bf01ae Compare August 9, 2023 15:04

github-actions bot removed the Component: C++ label Aug 9, 2023

pitrou reviewed Aug 9, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 9, 2023

lidavidm force-pushed the flight-async-py-client-getflightinfo branch from 1bf01ae to a89fe71 Compare August 9, 2023 19:10

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 9, 2023

lidavidm marked this pull request as ready for review August 9, 2023 19:11

lidavidm commented Aug 9, 2023

View reviewed changes

python/pyarrow/_flight.pyx Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 9, 2023

lidavidm force-pushed the flight-async-py-client-getflightinfo branch from a89fe71 to 68f2aa0 Compare August 9, 2023 19:25

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 9, 2023

lidavidm and others added 4 commits August 23, 2023 17:03

apacheGH-37093: [FlightRPC][Python] Add async GetFlightInfo

adab985

Bind to future instead of dealing with callbacks

9b286d5

Add async.h

8571b9f

Try to avoid type-unsafe void*

4154e4f

pitrou force-pushed the flight-async-py-client-getflightinfo branch from 0cc328e to 4154e4f Compare August 23, 2023 15:03

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 23, 2023

pitrou approved these changes Aug 23, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

Simplify AsyncioCall using a asyncio.Future

5c7d665

pitrou reviewed Aug 23, 2023

View reviewed changes

Forward async-support error message to Python, for informative test s…

58a938a

…kipping

github-actions bot added the Component: C++ label Aug 23, 2023

pitrou reviewed Aug 23, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 23, 2023

Mark override

07193d4

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 23, 2023

pitrou merged commit b6ab909 into apache:main Aug 23, 2023
32 of 33 checks passed

pitrou removed the awaiting change review Awaiting change review label Aug 23, 2023

lidavidm deleted the flight-async-py-client-getflightinfo branch August 23, 2023 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37093: [Python] Add async Flight client with GetFlightInfo #36986

GH-37093: [Python] Add async Flight client with GetFlightInfo #36986

lidavidm commented Aug 1, 2023 •

edited

Loading

This comment was marked as resolved.

lidavidm commented Aug 9, 2023

pitrou left a comment

pitrou Aug 9, 2023

lidavidm Aug 9, 2023

pitrou Aug 9, 2023

lidavidm Aug 9, 2023

lidavidm Aug 9, 2023

lidavidm commented Aug 9, 2023 •

edited

Loading

lidavidm commented Aug 10, 2023

lidavidm commented Aug 10, 2023

lidavidm commented Aug 10, 2023

pitrou commented Aug 10, 2023

lidavidm commented Aug 10, 2023

pitrou commented Aug 10, 2023

lidavidm commented Aug 10, 2023 •

edited

Loading

pitrou left a comment

This comment was marked as outdated.

This comment was marked as outdated.

pitrou Aug 23, 2023

pitrou commented Aug 23, 2023

pitrou commented Aug 23, 2023

pitrou commented Aug 23, 2023

pitrou Aug 23, 2023

lidavidm Aug 23, 2023

github-actions bot commented Aug 23, 2023

pitrou commented Aug 23, 2023

conbench-apache-arrow bot commented Aug 24, 2023

GH-37093: [Python] Add async Flight client with GetFlightInfo #36986

GH-37093: [Python] Add async Flight client with GetFlightInfo #36986

Conversation

lidavidm commented Aug 1, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

This comment was marked as resolved.

lidavidm commented Aug 9, 2023

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Aug 9, 2023 • edited Loading

lidavidm commented Aug 10, 2023

lidavidm commented Aug 10, 2023

lidavidm commented Aug 10, 2023

pitrou commented Aug 10, 2023

lidavidm commented Aug 10, 2023

pitrou commented Aug 10, 2023

lidavidm commented Aug 10, 2023 • edited Loading

pitrou left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

pitrou commented Aug 23, 2023

pitrou commented Aug 23, 2023

pitrou commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 23, 2023

pitrou commented Aug 23, 2023

conbench-apache-arrow bot commented Aug 24, 2023

lidavidm commented Aug 1, 2023 •

edited

Loading

lidavidm commented Aug 9, 2023 •

edited

Loading

lidavidm commented Aug 10, 2023 •

edited

Loading